-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate systemsettings test for KDE4-based and KF5-based #960
Conversation
54f44b4
to
0d54be3
Compare
looks ok but don't want to approve by myself as I don't know the use of the variables good enough. Please wait for others for their feedback |
my $self = shift; | ||
x11_start_program("systemsettings5", 6, {valid => 1}); | ||
if (get_var("LIVETEST")) { | ||
assert_screen 'test-systemsettings-1', 15; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if here would be just assert_screen with default timeout ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but is it wroth 30s here? especially in old systemsettings test that non-LIVETEST block is use 3s...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea what's the point of this 'if' actually. If the test fails in 3 or 30 seconds is not so important - it shouldn't fail at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is needle match, it will continue after 1 second
btw default timeout is In today ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know, as @coolo pointed in some other PR. When people start copy-pasting existing code IMO its better with defaults rather then arbitrary options valid only for some distant test.
Personally I think for assert_screen
always use default unless you need to wait longer. Shorter timeout will not speed up the test.
For check_screen
that's different and I see there are use cases for shorter timeouts and ideally there should be accompanying comment explaining why shorter timeout (\me going full @okurz ) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the road to madness :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, as I said above, I don't mind to use default timeout. and I don't know what is the current agreement/rule to treat timeout with assert_screen and check_screen.
oh, and yes, when I copy the new test from the old one I haven't aware I should change the content besides program name :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nilxam sysrich explained it pretty well in his PR #945 that is why I suggested to improve the documentation in https://progress.opensuse.org/issues/10380 so it will be made more clear.
that's the road to madness :)
@coolo according to your statements in the near past it seems there are many roads that lead to madness ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's true - mad, isn't it?
0d54be3
to
da9f225
Compare
In update test, there might have old KDE4 systemsettings as another candidate in krunner via auto-completion, therefore, separate systemsettings test to systemsettings(KDE4-based) and systemsettings5(KF5-based) test. openSUSE version less than or equal to 13.2 have to set KDE4 variable as 1, thus PLASMA5 variable won't be sets.
da9f225
to
6e86558
Compare
LGTM |
Separate systemsettings test for KDE4-based and KF5-based
In update test, there might have old KDE4 systemsettings as another candidate in krunner via auto-completion, therefore, separate systemsettings test to systemsettings(KDE4-based) and systemsettings5(KF5-based) test.
openSUSE version less than or equal to 13.2 have to set KDE4 variable as 1, thus PLASMA5 variable won't be sets.